chore: add minimal solhint config#13
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
kaze-cow
left a comment
kaze-cow
left a comment
There was a problem hiding this comment.
Lots of coding standards enforcement good to see.
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
Looks good, mostly minor comments.
Approving is blocked because there are changes belonging to PR 2 and I expect this to change once upstream PRs are merged.
|
Looks good! Can be merged once the previous PR is merged. |
anxolin
left a comment
anxolin
left a comment
There was a problem hiding this comment.
Looks mostly ok, but a few comments. The PR description says "and only adds the intentional rules", but I believe your config is not adding but replacing the rules
|
|
||
| ```shell | ||
| npm install --prefix dev | ||
| python -m venv dev/.venv |
There was a problem hiding this comment.
no need to change for Mac users, but just so you know, we need to use python3
| ```shell | ||
| npm install --prefix dev | ||
| python -m venv dev/.venv | ||
| dev/.venv/bin/pip install -r dev/requirements.txt |
There was a problem hiding this comment.
no such a file dev/requirements.txt
|
|
||
| ```shell | ||
| dev/node_modules/.bin/solhint --version | ||
| dev/.venv/bin/slither --version |
There was a problem hiding this comment.
Is it possible you are mixing things from another PR?
This PR is about solhint, but here you are stuff from slither, even though it explicitly says its out of scope
| forge fmt | ||
| ``` | ||
|
|
||
| ### Local tooling |
There was a problem hiding this comment.
I find the section a bit stange.
We have Build, Test, Format, so if you add solint makes sense in a section called Lint
Local tooling is also cast
| "description": "Local development dependencies for the contracts template.", | ||
| "license": "UNLICENSED", | ||
| "devDependencies": { | ||
| "solhint": "6.0.3" |
There was a problem hiding this comment.
why this version and not 6.2.1?
When I run the command I get this warning
| @@ -0,0 +1 @@ | |||
| {} | |||
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
I don't think this does what you intended.
You want to extend and override, right?
{ "extends": "../.solhint.json" }
| { | |
| { | |
| "extends": "../.solhint.json" , |
4 participants
Description
Add a small Solhint setup that uses the local Solhint dependency.
Context
The root Solhint config extends
solhint:recommendedand only adds the intentional rules. The script and test folders have small override configs for their own style.Out of Scope
This PR does not add Slither, Justfile commands, CI, or hooks.
Testing Instructions
npm install --prefix dev.dev/node_modules/.bin/solhint --max-warnings 0 'src/**/*.sol'.dev/node_modules/.bin/solhint --max-warnings 0 'script/**/*.sol'.dev/node_modules/.bin/solhint --max-warnings 0 'test/**/*.t.sol'.